Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MTG 1164 Apply showZeroBalance only for NFTs #368

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

n00m4d
Copy link
Contributor

@n00m4d n00m4d commented Jan 16, 2025

What

This PR adds condition to the JOIN SQL command which tells to join only rows with ast_owner_type field set to token. As a result from fungible_tokens table will be taken accounts which are related to fungible assets and not NFTs.

Also this PR adds new integration test which checks if that JOIN works as expected.

IMPORTANT

Do not merge into main until #376 is merged into this branch

Base automatically changed from fix/optimize-select-for-show_fungible-display-option to main January 17, 2025 11:49
@n00m4d n00m4d marked this pull request as ready for review January 20, 2025 11:57
Copy link
Collaborator

@armyhaylenko armyhaylenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LBTM

Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you! The only question is whether we need to modify any indexes

@n00m4d
Copy link
Contributor Author

n00m4d commented Jan 23, 2025

@StanChe nope, we should not. I've tested this query on nodes with Solana data and with that added condition it works same as without it. We are good there.

@n00m4d
Copy link
Contributor Author

n00m4d commented Jan 24, 2025

Double checked explain analyze for final query, these changes actually increases execution time, from >100ms to ~3 seconds. First tests I did showed incorrect results. So it's better not to merge this PR at the moment.

@n00m4d
Copy link
Contributor Author

n00m4d commented Jan 31, 2025

Latest commit adds improvements to SQL query. Also it adds 2 new indexes which also will improve selects.
Now it's ready for final review.

Copy link
Contributor

@StanChe StanChe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change, let's ask others to revalidate as well. The limit should be fixed

@@ -0,0 +1,3 @@
CREATE INDEX fungible_tokens_fbt_owner_fbt_asset_fbt_balance_idx ON fungible_tokens (fbt_owner, fbt_asset, fbt_balance);

CREATE INDEX assets_ast_owner_type_ast_pubkey_idx ON assets_v3 (ast_owner_type, ast_pubkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line please

@@ -0,0 +1,3 @@
CREATE INDEX fungible_tokens_fbt_owner_fbt_asset_fbt_balance_idx ON fungible_tokens (fbt_owner, fbt_asset, fbt_balance);

CREATE INDEX assets_ast_owner_type_ast_pubkey_idx ON assets_v3 (ast_owner_type, ast_pubkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may also want to drop assets_v3_owner_type as it becomes obsolete


query_builder.push(" LIMIT ");
if let Some(page_num) = page.filter(|&p| p > 1) {
let lim = (page_num - 1) * limit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition is wrong. it's page_num*limit. You want to have 2000 records pre filtered for the second page. Also add a test for the second page of pagination of this request. The correct way to do it is you always get a 1-based page and multiply it by the limit

query_builder.push(direction);

query_builder.push(" LIMIT ");
if let Some(page_num) = page.filter(|&p| p > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants